Skip to content

add github actions #349

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Feb 22, 2021
Merged

add github actions #349

merged 20 commits into from
Feb 22, 2021

Conversation

bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Feb 9, 2021

This adds a GitHub Actions CI, derived from a number of other ones in the community. This is a single workflow, with one tiny change to conf.py to avoid a long-deprecated API. I figure we can remove the travis job separately, maybe try to get RTD back up and running (might need to request the mamba feature flag).

I'm working directly off my master branch, where you can see my Many Commits of Shame with the occasional ✔️

Overview:

  • linux build job
    • uploads distribution artifacts
  • linux docs job
    • uploads docs artifacts
    • checks all links
      • 🆕 uses a link cache, expires every week
  • matrix test job (3 os x 4 pythons x 3 labs x 2 LTS nodejs)
    • downloads the distribution artifacts
    • runs nbval tests against one of them, depending on the python version
    • 🆕 coverage
    • 🆕 all excursions checks that the nbextension is packaged
    • 🆕 excursions with jupyterlab check that the labextension is packaged

Possible Follow-on

  • remove travis
  • README badges
  • not building the js stuff twice for sdist, wheel
  • linting/formatting (for non-generated code, only, maybe)
  • look into coverage misses, add some thresholds
  • add some more test stuff to extras
  • update sphinx plugin for traitlets 5 (super cool!), maybe spin off as separate package
  • better caching: having requirements.txt, yarn.lock, etc. would be needed to drive entropy

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Feb 9, 2021

For the full gory details of getting this up, see: bollwyvl#1

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Feb 9, 2021

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Feb 9, 2021

Nothing else planned here, probably ready for review!

@willingc
Copy link

willingc commented Feb 9, 2021

@bollwyvl RTD is already working with GH Webhooks and RTD preview rendering in ipywidgets. No GH action needed.

@bollwyvl
Copy link
Contributor Author

Welp, the travis one (was) running the link check, so figured I'd bring it across. Can certainly take out, if it's already being run on RTD.

@vidartf
Copy link
Member

vidartf commented Feb 11, 2021

AFAI remember, the lint check step done on travis also checks links in markdown files and examples notebooks (and not just the docs).

@bollwyvl
Copy link
Contributor Author

Run with linkchecks of everything:
https://github.com/bollwyvl/pythreejs/runs/1879733287

Of note: i did try checking the individual anchors with --check-anchors, and a few things (well, hundreds) fail, as the docs structure changed some, and it doesn't appear the upstream has versioned docs. Turns out a bunch of them still work, as some clever clientside stuff redirects them... lil' ol pytest isn't going to be able to follow them.

https://threejs.org/docs/#api/geometries/BoxGeometry     # magically redirects to...
https://threejs.org/docs/#api/en/geometries/BoxGeometry

...but it would be a bit of a haul to get everything fully accurate, and it could well break again in the future...

@vidartf
Copy link
Member

vidartf commented Feb 15, 2021

The upstream docs are unfortunately not versioned either, so some inaccuracies are bound to happen either way 😞

@bollwyvl
Copy link
Contributor Author

Sorry for the commit spam, thought it would be quick update to pull in the #342 changes! 🤷

Some findings:

  • the pyproject.toml build-system is basically incompatible with setupbase, as-is without hacks
    • it yields uninstallable sdists and can't be pip install . (-e might work... but...)
    • the workaround requires sys.path += [HERE] which is pretty much always a bad smell, but it works
  • once all that was working, the only big difference was a doubling of the up-front time to ~3min
    • installing lab is pretty expensive (a few upstreams don't have whls)
      • the pip cache helps a bit (probably 30s)
      • could potentially save another 30s by caching the yarn packages, but without a yarn.lock...
    • the lab 3 install (without node or lab build) is pretty much as fast as the sdist install, which makes sense
  • the link check cache wasn't doing quite what i wanted
    • now times out every week, but can pull from previous weeks, and update if needed

It would probably be good to get this in to shake down whatever else has to happen for a release...

@vidartf
Copy link
Member

vidartf commented Feb 19, 2021

the pyproject.toml build-system is basically incompatible with setupbase, as-is without hacks

Yes I've seen that locally as well. Removing the build_meta config value seems to work around that for now.

once all that was working, the only big difference was a doubling of the up-front time to ~3min

I'll give it a go to ensure that end-users are not going to see any increases. If the CI time is not being spent doing obvious "duplicate" work, I don't think this repo has a high enough activity level to spend too much time optimizing things.

installing lab is pretty expensive (a few upstreams don't have whls)

Yes, it would be good if the lab extension builder package could be a separate package with a minimal dependency tree, since it will be installed for all sdist installs as well. I also want to check that sdist installs work when node is not available on the system.

- name: Install package and docs dependencies
run: |
set -eux
pip install -vv -U .[docs,examples,test] requests_cache 'traitlets==4.*'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which bits are broken with traitlets 5? Would you mind linking to an issue, or creating a new one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, lotta force-pushin', hard to dig back up. It was something about one of the fancy enum traits...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made #351, got CI to show the issue.

Given there is no functional CI working, it would be lovely to get this merged so we're not having to look at a fork... likely we could iterate more effectively, and in parallel...

@bollwyvl
Copy link
Contributor Author

I also want to check that sdist installs work when node is not available on the system.

The 3.6 excursions all install from sdist, and have the assets:

https://github.com/bollwyvl/pythreejs/runs/1916098289?check_suite_focus=true#step:7:21581

@vidartf vidartf merged commit 8f9a7d5 into jupyter-widgets:master Feb 22, 2021
@bollwyvl
Copy link
Contributor Author

bollwyvl commented Feb 22, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants